sqlite: allow passing conflict resolution handler function #56352

louwers · 2024-12-24T15:30:12Z

Resolves #56210 by allowing to pass a conflict resolution handler that returns a conflict resolution type instead of just a conflict resolution type.

As @Renegade334 points out in that issue, it does not make sense to pass SQLITE_CHANGESET_REPLACE because this conflict resolution type is only valid for certain conflict types. Therefore an API where the conflict-resolution handler receives the conflict type makes sense, so the user can return a valid conflict resolution type for a particular conflict type.

An API where an individual (conflicting) change can be inspected (as is possible with the SQLite C conflict handler) would be a good follow-up. This would then be added as a second argument to the conflict handler.

Note that this is a breaking change, but since the SQLite module is experimental I don't think that is an issue.

Note: some of the documentation I added comes pretty much verbatim from the SQLite Documentation, but this is not an issue because the SQLite documentation is dedicated to the public domain. Also I linked to the SQLite documentation.

Renegade334

Just a couple of quick observations about callback handling.

src/node_sqlite.cc

codecov · 2024-12-24T17:27:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (4fd5db4) to head (5e39d22).
Report is 32 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #56352    +/-   ##
========================================
  Coverage   88.54%   88.54%            
========================================
  Files         657      657            
  Lines      190395   190718   +323     
  Branches    36552    36601    +49     
========================================
+ Hits       168586   168877   +291     
- Misses      14992    15021    +29     
- Partials     6817     6820     +3

Files with missing lines	Coverage Δ
src/node_sqlite.cc	`82.34% <100.00%> (+0.54%)`	⬆️

... and 41 files with indirect coverage changes

src/node_sqlite.cc

louwers · 2024-12-24T21:10:29Z

@cjihrig Could you run CI?

nodejs-github-bot · 2024-12-24T21:16:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64199/

louwers · 2024-12-25T23:24:24Z

Looks like a flaky test. Needs a re-run I think.

nodejs-github-bot · 2024-12-25T23:51:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64212/

nodejs-github-bot · 2024-12-26T07:29:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64219/

src/node_sqlite.cc

doc/api/sqlite.md

src/node_sqlite.cc

test/parallel/test-sqlite-session.js

src/node_sqlite.cc

…dler

louwers · 2024-12-28T22:03:07Z

@cjihrig Thanks for the review. Could you trigger another CI run?

nodejs-github-bot · 2024-12-29T01:13:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64245/

nodejs-github-bot · 2024-12-29T16:12:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64252/

nodejs-github-bot · 2024-12-29T17:37:56Z

Landed in 0dbbaba

PR-URL: #56352 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | minor | `23.5.0` -> `23.6.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>nodejs/node (node)</summary> ### [`v23.6.0`](https://github.com/nodejs/node/releases/tag/v23.6.0): 2025-01-07, Version 23.6.0 (Current), @marco-ippolito [Compare Source](nodejs/node@v23.5.0...v23.6.0) ##### Notable Changes ##### Unflagging --experimental-strip-types This release enables the flag `--experimental-strip-types` by default. Node.js will be able to execute TypeScript files without additional configuration: ```bash node file.ts ``` There are some limitations in the supported syntax documented at <https://nodejs.org/api/typescript.html#type-stripping> This feature is experimental and is subject to change. Contributed by Marco Ippolito in [#56350](nodejs/node#56350) ##### Other Notable Changes - \[[`c1023284c3`](nodejs/node@c1023284c3)] - **(SEMVER-MINOR)** **lib**: add typescript support to STDIN eval (Marco Ippolito) [#56359](nodejs/node#56359) - \[[`8dc39e5e2e`](nodejs/node@8dc39e5e2e)] - **(SEMVER-MINOR)** **process**: add process.ref() and process.unref() methods (James M Snell) [#56400](nodejs/node#56400) - \[[`8b20cc212b`](nodejs/node@8b20cc212b)] - **(SEMVER-MINOR)** **worker**: add eval ts input (Marco Ippolito) [#56394](nodejs/node#56394) ##### Commits - \[[`7b4d288116`](nodejs/node@7b4d288116)] - **assert**: make partialDeepStrictEqual throw when comparing \[0] with \[-0] (Giovanni) [#56237](nodejs/node#56237) - \[[`0ec2ed0a0b`](nodejs/node@0ec2ed0a0b)] - **build**: fix GN build for ngtcp2 (Cheng) [#56300](nodejs/node#56300) - \[[`ab3e64630b`](nodejs/node@ab3e64630b)] - **build**: test macos-13 on GitHub actions (Michaël Zasso) [#56307](nodejs/node#56307) - \[[`46fb69daca`](nodejs/node@46fb69daca)] - **build**: build v8 with -fvisibility=hidden on macOS (Joyee Cheung) [#56275](nodejs/node#56275) - \[[`9d4930b993`](nodejs/node@9d4930b993)] - **deps**: update simdutf to 5.7.2 (Node.js GitHub Bot) [#56388](nodejs/node#56388) - \[[`6afe36397e`](nodejs/node@6afe36397e)] - **deps**: update amaro to 0.2.1 (Node.js GitHub Bot) [#56390](nodejs/node#56390) - \[[`195990a0ee`](nodejs/node@195990a0ee)] - **deps**: update googletest to [`7d76a23`](nodejs/node@7d76a23) (Node.js GitHub Bot) [#56387](nodejs/node#56387) - \[[`b9c0852fc6`](nodejs/node@b9c0852fc6)] - **deps**: update googletest to [`e54519b`](nodejs/node@e54519b) (Node.js GitHub Bot) [#56370](nodejs/node#56370) - \[[`eaefd90128`](nodejs/node@eaefd90128)] - **deps**: update ngtcp2 to 1.10.0 (Node.js GitHub Bot) [#56334](nodejs/node#56334) - \[[`06de0c65cf`](nodejs/node@06de0c65cf)] - **deps**: update simdutf to 5.7.0 (Node.js GitHub Bot) [#56332](nodejs/node#56332) - \[[`03df76cdec`](nodejs/node@03df76cdec)] - **doc**: add example for piping ReadableStream (Gabriel Schulhof) [#56415](nodejs/node#56415) - \[[`38ce249b07`](nodejs/node@38ce249b07)] - **doc**: expand description of `parseArg`'s `default` (Kevin Gibbons) [#54431](nodejs/node#54431) - \[[`ecc718cef2`](nodejs/node@ecc718cef2)] - **doc**: use `<ul>` instead of `<ol>` in `SECURITY.md` (Antoine du Hamel) [#56346](nodejs/node#56346) - \[[`3db4809130`](nodejs/node@3db4809130)] - **doc**: clarify that WASM is trusted (Matteo Collina) [#56345](nodejs/node#56345) - \[[`384ccbacd5`](nodejs/node@384ccbacd5)] - **doc**: update macOS and Xcode versions for releases (Michaël Zasso) [#56337](nodejs/node#56337) - \[[`3943986e88`](nodejs/node@3943986e88)] - **doc**: fix the `crc32` documentation (Kevin Toshihiro Uehara) [#55898](nodejs/node#55898) - \[[`710b8fc6ed`](nodejs/node@710b8fc6ed)] - **doc**: add entry to changelog about SQLite Session Extension (Bart Louwers) [#56318](nodejs/node#56318) - \[[`4c978b4d77`](nodejs/node@4c978b4d77)] - **doc**: fix links in `module.md` (Antoine du Hamel) [#56283](nodejs/node#56283) - \[[`cdb631efe7`](nodejs/node@cdb631efe7)] - **esm**: add experimental support for addon modules (Chengzhong Wu) [#55844](nodejs/node#55844) - \[[`db83d2f0ee`](nodejs/node@db83d2f0ee)] - ***Revert*** "**events**: add hasEventListener util for validate" (origranot) [#56282](nodejs/node#56282) - \[[`c2baae84ce`](nodejs/node@c2baae84ce)] - **lib**: refactor execution.js (Marco Ippolito) [#56358](nodejs/node#56358) - \[[`c1023284c3`](nodejs/node@c1023284c3)] - **(SEMVER-MINOR)** **lib**: add typescript support to STDIN eval (Marco Ippolito) [#56359](nodejs/node#56359) - \[[`e4b795ec4a`](nodejs/node@e4b795ec4a)] - **lib**: optimize `prepareStackTrace` on builtin frames (Chengzhong Wu) [#56299](nodejs/node#56299) - \[[`d1b009b623`](nodejs/node@d1b009b623)] - **lib**: suppress source map lookup exceptions (Chengzhong Wu) [#56299](nodejs/node#56299) - \[[`c2837f0805`](nodejs/node@c2837f0805)] - **meta**: move one or more collaborators to emeritus (Node.js GitHub Bot) [#56342](nodejs/node#56342) - \[[`72336233f2`](nodejs/node@72336233f2)] - **meta**: move MoLow to TSC regular member (Moshe Atlow) [#56276](nodejs/node#56276) - \[[`4f77920a9d`](nodejs/node@4f77920a9d)] - **module**: fix async resolution error within the sync `findPackageJSON` (Jacob Smith) [#56382](nodejs/node#56382) - \[[`e5ba216501`](nodejs/node@e5ba216501)] - **(SEMVER-MINOR)** **module**: unflag --experimental-strip-types (Marco Ippolito) [#56350](nodejs/node#56350) - \[[`959f133a22`](nodejs/node@959f133a22)] - **module**: support eval with ts syntax detection (Marco Ippolito) [#56285](nodejs/node#56285) - \[[`717cfa4fac`](nodejs/node@717cfa4fac)] - **module**: use buffer.toString base64 (Chengzhong Wu) [#56315](nodejs/node#56315) - \[[`c2f4d8d688`](nodejs/node@c2f4d8d688)] - **node-api**: define version 10 (Gabriel Schulhof) [#55676](nodejs/node#55676) - \[[`417a8ebdec`](nodejs/node@417a8ebdec)] - **node-api**: remove deprecated attribute from napi_module_register (Vladimir Morozov) [#56162](nodejs/node#56162) - \[[`8dc39e5e2e`](nodejs/node@8dc39e5e2e)] - **(SEMVER-MINOR)** **process**: add process.ref() and process.unref() methods (James M Snell) [#56400](nodejs/node#56400) - \[[`d194f1ab5f`](nodejs/node@d194f1ab5f)] - **sqlite**: pass conflict type to conflict resolution handler (Bart Louwers) [#56352](nodejs/node#56352) - \[[`29f5d70452`](nodejs/node@29f5d70452)] - **src**: use v8::LocalVector consistently with other minor cleanups (James M Snell) [#56417](nodejs/node#56417) - \[[`2a5543b78e`](nodejs/node@2a5543b78e)] - **src**: use starts_with in fs_permission.cc (ishabi) [#55811](nodejs/node#55811) - \[[`3a3f5c9a64`](nodejs/node@3a3f5c9a64)] - **stream**: validate undefined sizeAlgorithm in WritableStream (Jason Zhang) [#56067](nodejs/node#56067) - \[[`6e6f6b071a`](nodejs/node@6e6f6b071a)] - **test**: add ts eval snapshots (Marco Ippolito) [#56358](nodejs/node#56358) - \[[`8a87e39052`](nodejs/node@8a87e39052)] - **test**: remove empty lines from snapshots (Marco Ippolito) [#56358](nodejs/node#56358) - \[[`510649f617`](nodejs/node@510649f617)] - **test**: use unusual chars in the path to ensure our tests are robust (Antoine du Hamel) [#48409](nodejs/node#48409) - \[[`54f6d681a0`](nodejs/node@54f6d681a0)] - **test**: remove flaky designation (Luigi Pinca) [#56369](nodejs/node#56369) - \[[`20ace0bb01`](nodejs/node@20ace0bb01)] - **test**: remove test-worker-arraybuffer-zerofill flaky designation (Luigi Pinca) [#56364](nodejs/node#56364) - \[[`b757e40525`](nodejs/node@b757e40525)] - **test**: remove test-net-write-fully-async-hex-string flaky designation (Luigi Pinca) [#56365](nodejs/node#56365) - \[[`64556baddc`](nodejs/node@64556baddc)] - **test**: improve abort signal dropping test (Edy Silva) [#56339](nodejs/node#56339) - \[[`accbdad329`](nodejs/node@accbdad329)] - **test**: enable ts test on win arm64 (Marco Ippolito) [#56349](nodejs/node#56349) - \[[`4188ee00d1`](nodejs/node@4188ee00d1)] - **test**: deflake test-watch-file-shared-dependency (Luigi Pinca) [#56344](nodejs/node#56344) - \[[`079cee0609`](nodejs/node@079cee0609)] - **test**: skip `test-sqlite-extensions` when SQLite is not built by us (Antoine du Hamel) [#56341](nodejs/node#56341) - \[[`96a38044ee`](nodejs/node@96a38044ee)] - **test**: increase spin for eventloop test on s390 (Michael Dawson) [#56228](nodejs/node#56228) - \[[`c062ffc242`](nodejs/node@c062ffc242)] - **test**: add coverage for pipeline (jakecastelli) [#56278](nodejs/node#56278) - \[[`d4404f0d0e`](nodejs/node@d4404f0d0e)] - **test**: migrate message eval tests from Python to JS (Yiyun Lei) [#50482](nodejs/node#50482) - \[[`9369942745`](nodejs/node@9369942745)] - **test**: check typescript loader (Marco Ippolito) [#54657](nodejs/node#54657) - \[[`4930244484`](nodejs/node@4930244484)] - **test**: remove async-hooks/test-writewrap flaky designation (Luigi Pinca) [#56048](nodejs/node#56048) - \[[`7819bfec69`](nodejs/node@7819bfec69)] - **test**: deflake test-esm-loader-hooks-inspect-brk (Luigi Pinca) [#56050](nodejs/node#56050) - \[[`e9762bf005`](nodejs/node@e9762bf005)] - **test**: add test case for listeners (origranot) [#56282](nodejs/node#56282) - \[[`c1627e9d19`](nodejs/node@c1627e9d19)] - **test**: make `test-permission-sqlite-load-extension` more robust (Antoine du Hamel) [#56295](nodejs/node#56295) - \[[`97d854e1d5`](nodejs/node@97d854e1d5)] - **test_runner,cli**: mark test isolation as stable (Colin Ihrig) [#56298](nodejs/node#56298) - \[[`a4f336fdd4`](nodejs/node@a4f336fdd4)] - **tools**: fix `require-common-first` lint rule from subfolder (Antoine du Hamel) [#56325](nodejs/node#56325) - \[[`dc3dafcb50`](nodejs/node@dc3dafcb50)] - **tools**: add release line label when opening release proposal (Antoine du Hamel) [#56317](nodejs/node#56317) - \[[`2a5ac932ac`](nodejs/node@2a5ac932ac)] - **url**: use resolved path to convert UNC paths to URL (Antoine du Hamel) [#56302](nodejs/node#56302) - \[[`8b20cc212b`](nodejs/node@8b20cc212b)] - **(SEMVER-MINOR)** **worker**: add eval ts input (Marco Ippolito) [#56394](nodejs/node#56394) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Dec 24, 2024

louwers force-pushed the sqlite-conflict-handler branch 4 times, most recently from 552a1e8 to 933ce21 Compare December 24, 2024 16:11

sqlite: pass conflict type to conflict resolution handler

3635cd9

louwers force-pushed the sqlite-conflict-handler branch from 933ce21 to 3635cd9 Compare December 24, 2024 16:11

louwers changed the title ~~sqlite: pass conflict type to conflict-resulution handler~~ sqlite: pass conflict type to conflict resolution handler Dec 24, 2024

Renegade334 reviewed Dec 24, 2024

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved

src/node_sqlite.cc Outdated Show resolved Hide resolved

louwers added 2 commits December 24, 2024 19:45

sqlite: improve error message when applying changeset fails

e6001b3

sqlite: handle exception thrown in conflict handler

a06a068

louwers force-pushed the sqlite-conflict-handler branch from f63c3b4 to a06a068 Compare December 24, 2024 20:58

louwers commented Dec 24, 2024

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2024

louwers changed the title ~~sqlite: pass conflict type to conflict resolution handler~~ sqlite: allow passing conflict resolution handler function Dec 24, 2024

louwers requested a review from Renegade334 December 25, 2024 19:38

cjihrig approved these changes Dec 28, 2024

View reviewed changes

src/node_sqlite.cc Outdated Show resolved Hide resolved